Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(complete): Harden --target completions #14564

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 18, 2024

What does this PR try to resolve?

I found --target wasn't working with rustup as I expected, so this fixes it. We generally only reference rustup if we are running under it but I think in UX code like this, with a fallback scheme, this should be reasonable enough.

How should we test and review this PR?

Additional information

…ions

If there is any problem with rustup, we should fallback to rustc.

(this also removes some extra allocations)
clap-rs/clap#5733 removed the rustup proxy so that
`CARGO_COMPLETE=bash cargo +nightly` works
(with a side benefit of removing the proxy overhead).

As a downside, cargo no longer knows it is running within rustup, so we
aren't reading `--target` candidates from rustup.

This changes the code to always try rustup.  It is likely a good enough
source, even if the user isn't currently using it.
The candidates should be about the same, just rustup hides some by
default.
Hiding just means it isn't shown by default but if only hidden
candidates match, then we show them.
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2024
@Rustin170506
Copy link
Member

I found --target wasn't working with rustup as I expected, so this fixes it.

May I ask what your expectations are here?

@epage
Copy link
Contributor Author

epage commented Sep 18, 2024

Its further described in b9fc8d4

clap-rs/clap#5733 removed the rustup proxy so that
CARGO_COMPLETE=bash cargo +nightly works
(with a side benefit of removing the proxy overhead).

As a downside, cargo no longer knows it is running within rustup, so we
aren't reading --target candidates from rustup.

This changes the code to always try rustup. It is likely a good enough
source, even if the user isn't currently using it.
The candidates should be about the same, just rustup hides some by
default.
Hiding just means it isn't shown by default but if only hidden
candidates match, then we show them.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was my bash too old or something, anyway it is fixed and working. Thanks!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 19, 2024

📌 Commit a5c25f3 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2024
@bors
Copy link
Collaborator

bors commented Sep 19, 2024

⌛ Testing commit a5c25f3 with merge eaee77d...

@bors
Copy link
Collaborator

bors commented Sep 19, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing eaee77d to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Sep 19, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing eaee77d to master...

@bors bors merged commit eaee77d into rust-lang:master Sep 19, 2024
22 checks passed
@bors
Copy link
Collaborator

bors commented Sep 19, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@epage epage deleted the rustup branch September 19, 2024 21:56
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2024
Update cargo

16 commits in a9a418d1a22f29e7dfd034e3b93f15657e608a29..eaee77dc1584be45949b75e4c4c9a841605e3a4b
2024-09-15 19:13:12 +0000 to 2024-09-19 21:10:23 +0000
- fix(complete): Harden `--target` completions (rust-lang/cargo#14564)
- Cleanup duplicated check-cfg lint logic (rust-lang/cargo#14567)
- Revert "remove reference to incomplete crates.io feature from docs" (rust-lang/cargo#14562)
- feat: Add custom completer for `cargo help <TAB>` (rust-lang/cargo#14557)
- docs(unstable): Expand on completion documentation (rust-lang/cargo#14563)
- feat: Add custom completer for `cargo build --example=<TAB>` (rust-lang/cargo#14531)
- remove reference to incomplete crates.io feature from docs (rust-lang/cargo#14561)
- fix(complete): Fix problems on my machine (rust-lang/cargo#14558)
- feat: Add custom completer for completing benchmark names (rust-lang/cargo#14532)
- refactor(info): Use the `shell.note` to print the note (rust-lang/cargo#14554)
- feat: Add custom completer for completing test names (rust-lang/cargo#14548)
- Suggest `cargo info` command in the `cargo search` result (rust-lang/cargo#14537)
- feat: Add custom completer for completing target triple (rust-lang/cargo#14535)
- feat: Add custom completer for `cargo -Z <TAB>` (rust-lang/cargo#14536)
- feat: Add custom completer for completing installed binaries (rust-lang/cargo#14534)
- feat: Add custom completer for completing bin names (rust-lang/cargo#14533)

r? ghost
@rustbot rustbot added this to the 1.83.0 milestone Sep 21, 2024
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Sep 21, 2024
Update cargo

16 commits in a9a418d1a22f29e7dfd034e3b93f15657e608a29..eaee77dc1584be45949b75e4c4c9a841605e3a4b
2024-09-15 19:13:12 +0000 to 2024-09-19 21:10:23 +0000
- fix(complete): Harden `--target` completions (rust-lang/cargo#14564)
- Cleanup duplicated check-cfg lint logic (rust-lang/cargo#14567)
- Revert "remove reference to incomplete crates.io feature from docs" (rust-lang/cargo#14562)
- feat: Add custom completer for `cargo help <TAB>` (rust-lang/cargo#14557)
- docs(unstable): Expand on completion documentation (rust-lang/cargo#14563)
- feat: Add custom completer for `cargo build --example=<TAB>` (rust-lang/cargo#14531)
- remove reference to incomplete crates.io feature from docs (rust-lang/cargo#14561)
- fix(complete): Fix problems on my machine (rust-lang/cargo#14558)
- feat: Add custom completer for completing benchmark names (rust-lang/cargo#14532)
- refactor(info): Use the `shell.note` to print the note (rust-lang/cargo#14554)
- feat: Add custom completer for completing test names (rust-lang/cargo#14548)
- Suggest `cargo info` command in the `cargo search` result (rust-lang/cargo#14537)
- feat: Add custom completer for completing target triple (rust-lang/cargo#14535)
- feat: Add custom completer for `cargo -Z <TAB>` (rust-lang/cargo#14536)
- feat: Add custom completer for completing installed binaries (rust-lang/cargo#14534)
- feat: Add custom completer for completing bin names (rust-lang/cargo#14533)

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants